Fix jarsigner PKIX error for non-exportable AKV certificates via AIA chain completion#47977
Conversation
Co-authored-by: rujche <171773178+rujche@users.noreply.github.com>
Co-authored-by: rujche <171773178+rujche@users.noreply.github.com>
Co-authored-by: rujche <171773178+rujche@users.noreply.github.com>
Co-authored-by: rujche <171773178+rujche@users.noreply.github.com>
|
Hi @copilot. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
…ficates When a non-exportable Azure Key Vault certificate is used with jarsigner, the /secrets/ endpoint returns only the leaf certificate (no intermediate CAs). This causes jarsigner -verify to fail with: PKIX path building failed: unable to find valid certification path to requested target Fix: after loading certificates from the AKV secret bundle, walk the chain upward from the current top. If the top cert is not self-signed (i.e. the chain is incomplete), parse the AIA (Authority Information Access) extension (OID 1.3.6.1.5.5.7.1.1) to find the CA Issuers URL and download the missing intermediate CA certificate. Repeat until the chain reaches a self-signed root CA. Changes: - CertificateUtil: add completeChainViaAia() and downloadIssuerCertificateFromAia() methods; call completeChainViaAia() from loadCertificatesFromSecretBundleValue() after ordering - HttpUtil: add getBytes(String url) for binary (DER) certificate downloads - AiaCertificateChainTest: 10 unit tests including two PKIX path-building tests that reproduce the exact reported error and confirm it is resolved Fixes: #44267
rujche
changed the title
- Fix maxDownloads counter to only decrement on actual HTTP downloads, not during certificate reordering. This prevents premature loop exit when reorganizing existing issuer certificates. - Replace string concatenation with parameterized logging in HttpUtil.getBytes() to satisfy GoodLoggingCheck. - Restore GoodLoggingCheck suppressions for CertificateUtil.java and HttpUtil.java (module requires java.util.logging for JCA provider bootstrap). Addresses: maxDownloads safety limit, GoodLoggingCheck violation in exception logging
… system property pollution
…xception, fix comments
…xception, fix comments
vcolin7
left a comment
vcolin7
left a comment
There was a problem hiding this comment.
The changes look good for an initial review, but there are a few places where I think we can make improvements. If you have any questions, I'd be happy to discuss my comments further :)
| // Complete the chain by downloading any missing intermediate CA certificates via the AIA extension. | ||
| // This handles the case where only the leaf certificate was stored in Azure Key Vault | ||
| // (e.g. a non-exportable certificate where the caller only merged the leaf cert during CSR completion). | ||
| certificates = completeChainViaAia(certificates); |
There was a problem hiding this comment.
The way the code is written, this will run on every KeyVaultClient.getCertificateChain() call, not just when jarsigner is used (like when using TLS in Spring Boot / App Service); meaning that completeChainViaAia() will now issue an outbound HTTP call on every certificate load and on every refresh interval, turning a previously offline, in-memory operation into a network-dependent one by default (up to 10s per missing link).
The self-signed short-circuit and having an opt-out property mitigate this, but enabling it by default for the entire TLS path is a meaningful behavioral change. @rujche Can we confirm this is an intentional decision?
Two alternatives worth considering: (a) only complete the chain when the loaded chain has a single cert (the true leaf-only, non-exportable case that #44267 describes), or (b) make completion opt-in for the TLS path and keep it default-on only where signing chains are needed.
| * @param orderedCertificates certificate array with contiguous issuer path + any unplaced certs appended | ||
| * @return the (potentially extended) certificate array with missing intermediates inserted in the valid chain | ||
| */ | ||
| static Certificate[] completeChainViaAia(Certificate[] orderedCertificates) { |
There was a problem hiding this comment.
Right now, completeChainViaAia() re-downloads issuer certs from the CA's AIA endpoint on every incomplete chain load, and again on every certificates-refresh-interval cycle, for every alias that shares an issuer. Since issuer/intermediate/root certs are effectively immutable, we'd have repeated/avoidable latency and a runtime dependency on public CA endpoints (e.g. cacerts.digicert.com), which can become noticeable at scale (many instances/aliases x refresh cycles hitting the same few URLs with their own throttling rules/rate-limits).
I think it's worth considering using a small static in-memory cache using the CA Issuers URL (or issuer subject DN) as keys to eliminate said repeated roundtrips. If you'd prefer to keep this PR focused on the core fix, we can do so in a separate PR.
To keep the cache safe, I'd recommend the following:
-
Caching the bytes, not the validation. We'd only store the parsed/validated issuer cert and still run
isValidIssuer(...)(signature + CA +keyCertSign) against the current certificate on every use whether it's in the cache or not. This way, we'd skip the HTTP GET on a hit, but never the crypto validation. Otherwise, a malicious leaf pointing at a URL with a cached-but-wrong issuer could shortcut verification. -
Limiting the cache size. We'd use a size cap (like a bounded
ConcurrentHashMapor similar) and ideally a TTL, so a cert advertising many distinct AIA URLs can't grow the map without limit (memory-pressure DoS) and a reissued/revoked intermediate isn't served stale indefinitely.
| if (validIssuerInChain != null) { | ||
| // Issuer exists in the chain. If it's not in the expected position (validChainEnd+1), | ||
| // move it to make the chain contiguous | ||
| if (validIssuerIndex != validChainEnd + 1) { | ||
| LOGGER.log(FINE, "Valid issuer found but not at contiguous position. Moving from index {0} to {1}.", | ||
| new Object[] { validIssuerIndex, validChainEnd + 1 }); | ||
| chain.remove(validIssuerIndex); | ||
| chain.add(validChainEnd + 1, validIssuerInChain); | ||
| } else { | ||
| LOGGER.log(FINE, "Valid issuer already at correct contiguous position."); | ||
| } | ||
| // Continue the loop to potentially download the issuer's issuer | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The maxDownloads safety counter is only decremented when downloading (on line 406), but the branch that repositions a valid issuer already present in the chain ends in continue without decrementing the count, so loop termination there depends entirely on findValidChainEnd() advancing on every iteration. It seems the normal cases should terminate, but a malformed or adversarial input set (e.g. having the same intermediate issue twice in the chain, or a re-issued / cross-signed intermediate that shares subject DN + key) could theoretically keep the loop spinning.
I'd recommend adding a hard iteration cap on the outer while (true) (independent of maxDownloads) as a defensive guard, so termination doesn't rely on the reposition logic always making forward progress.
There was a problem hiding this comment.
Note: This PR adds 500+ lines of security-sensitive PKI logic (chain ordering, AIA resolution, issuer validation, self-signed detection) to a single util class as part of a bugfix. The code is well-structured and thoroughly tested, so I wouldn't consider my comment a blocker, but it's worth pointing out there's a substantial amount of new crypto surface to own and maintain. It's worth considering whether the AIA-completion logic belongs in its own dedicated class (e.g. AiaChainCompleter) to keep CertificateUtil focused and make the security-critical path easier to review and test in isolation.
| private static final Logger LOGGER = Logger.getLogger(CertificateUtil.class.getName()); | ||
| private static final String BEGIN_CERTIFICATE = "-----BEGIN CERTIFICATE-----"; | ||
| private static final String END_CERTIFICATE = "-----END CERTIFICATE-----"; | ||
| static final String DISABLE_AIA_DOWNLOAD_PROPERTY = "azure.keyvault.jca.disable-aia-download"; |
There was a problem hiding this comment.
Can we add a short Javadoc note here explaining what this property does like we did in the CHANGELOG/README? It might help maintainers in the future :)
| } catch (GeneralSecurityException e) { | ||
| // If signature verification fails or any error occurs, it's not a valid issuer | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Exception handling:
| } catch (GeneralSecurityException e) { | |
| // If signature verification fails or any error occurs, it's not a valid issuer | |
| return false; | |
| } | |
| } catch (CertificateExpiredException | CertificateNotYetValidException e) { | |
| LOGGER.log(FINE, "Issuer certificate [{0}] is expired or not yet valid; rejecting it as an issuer.", | |
| issuer.getSubjectX500Principal().getName()); | |
| return false; | |
| } catch (GeneralSecurityException e) { | |
| // If signature verification fails or any other error occurs, it's not a valid issuer | |
| return false; | |
| } |
You'd also need to add the following import statements:
import java.security.cert.CertificateExpiredException;
...
import java.security.cert.CertificateNotYetValidException;| assertEquals(leafWithBadIssuerAia, result[0]); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd add a test where we build an issuer that is a valid CA in every respect EXCEPT that it has already expired for isValidIssuer() to reject via checkValidity().
| @Test | |
| void completeChainViaAiaRejectsExpiredIssuer() throws Exception { | |
| KeyPairGenerator keyGen = KeyPairGenerator.getInstance("RSA"); | |
| keyGen.initialize(2048); | |
| // Build an issuer that is a valid CA in every respect EXCEPT that it has already expired. | |
| // isValidIssuer must reject it via checkValidity() so it is never inserted into the chain. | |
| Date expiredNotBefore = new Date(System.currentTimeMillis() - 86_400_000L * 30); | |
| Date expiredNotAfter = new Date(System.currentTimeMillis() - 86_400_000L); | |
| KeyPair expiredIssuerKeyPair = keyGen.generateKeyPair(); | |
| X509Certificate expiredIssuerCert | |
| = buildCertificate(expiredIssuerKeyPair.getPublic(), "CN=Expired Issuer", "CN=Expired Issuer", | |
| expiredIssuerKeyPair.getPrivate(), true, null, KeyUsage.keyCertSign, expiredNotBefore, expiredNotAfter); | |
| KeyPair leafKeyPair = keyGen.generateKeyPair(); | |
| X509Certificate leafWithExpiredAia = buildCertificate(leafKeyPair.getPublic(), "CN=Leaf", "CN=Expired Issuer", | |
| expiredIssuerKeyPair.getPrivate(), false, AIA_BAD_ISSUER_URL); | |
| try (MockedStatic<HttpUtil> httpMock = Mockito.mockStatic(HttpUtil.class)) { | |
| httpMock.when(() -> HttpUtil.getBytes(AIA_BAD_ISSUER_URL)).thenReturn(expiredIssuerCert.getEncoded()); | |
| Certificate[] result = CertificateUtil.completeChainViaAia(new Certificate[] { leafWithExpiredAia }); | |
| assertEquals(1, result.length, | |
| "An expired issuer certificate must be rejected and not inserted into the chain"); | |
| assertEquals(leafWithExpiredAia, result[0]); | |
| } | |
| } | |
| private static X509Certificate buildCertificate(java.security.PublicKey subjectPublicKey, String subjectDn, | ||
| String issuerDn, PrivateKey signingKey, boolean isCa, String aiaUrl, Integer keyUsageFlags) throws Exception { | ||
|
|
||
| X500Name subject = new X500Name(subjectDn); | ||
| X500Name issuer = new X500Name(issuerDn); | ||
| Date notBefore = new Date(System.currentTimeMillis() - 86_400_000L); | ||
| Date notAfter = new Date(System.currentTimeMillis() + 86_400_000L * 365); |
There was a problem hiding this comment.
Adding overload to use in the proposed new test.
| private static X509Certificate buildCertificate(java.security.PublicKey subjectPublicKey, String subjectDn, | |
| String issuerDn, PrivateKey signingKey, boolean isCa, String aiaUrl, Integer keyUsageFlags) throws Exception { | |
| X500Name subject = new X500Name(subjectDn); | |
| X500Name issuer = new X500Name(issuerDn); | |
| Date notBefore = new Date(System.currentTimeMillis() - 86_400_000L); | |
| Date notAfter = new Date(System.currentTimeMillis() + 86_400_000L * 365); | |
| private static X509Certificate buildCertificate(java.security.PublicKey subjectPublicKey, String subjectDn, | |
| String issuerDn, PrivateKey signingKey, boolean isCa, String aiaUrl, Integer keyUsageFlags) throws Exception { | |
| Date notBefore = new Date(System.currentTimeMillis() - 86_400_000L); | |
| Date notAfter = new Date(System.currentTimeMillis() + 86_400_000L * 365); | |
| return buildCertificate(subjectPublicKey, subjectDn, issuerDn, signingKey, isCa, aiaUrl, keyUsageFlags, | |
| notBefore, notAfter); | |
| } | |
| private static X509Certificate buildCertificate(java.security.PublicKey subjectPublicKey, String subjectDn, | |
| String issuerDn, PrivateKey signingKey, boolean isCa, String aiaUrl, Integer keyUsageFlags, Date notBefore, | |
| Date notAfter) throws Exception { | |
| X500Name subject = new X500Name(subjectDn); | |
| X500Name issuer = new X500Name(issuerDn); |
| List<Certificate> chain = new ArrayList<>(Arrays.asList(orderedCertificates)); | ||
| int maxDownloads = 10; // Safety limit to prevent infinite loops | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
| while (true) { | |
| // Hard limit on outer iterations, independent of maxDownloads. Repositioning below can take | |
| // `continue` without decrementing maxDownloads, so overall termination must not rely solely on | |
| // findValidChainEnd() moving the index forward. Malformed or adversarial input (e.g. duplicate or | |
| // cross-signed intermediate issuers sharing a subject DN and key) could otherwise keep the reposition | |
| // logic looping indefinitely. The bound is intentionally generous so it never truncates a legitimately | |
| // completable chain. | |
| int remainingIterations = 4 * (chain.size() + maxDownloads) + 16; | |
| while (true) { | |
| if (--remainingIterations < 0) { | |
| LOGGER.log(FINE, "Reached maximum certificate chain-completion iterations. Stopping to guard against " | |
| + "non-terminating input (possible duplicate or cross-signed intermediates)."); | |
| break; | |
| } |
| if (validIssuerInChain != null) { | ||
| // Issuer exists in the chain. If it's not in the expected position (validChainEnd+1), | ||
| // move it to make the chain contiguous | ||
| if (validIssuerIndex != validChainEnd + 1) { | ||
| LOGGER.log(FINE, "Valid issuer found but not at contiguous position. Moving from index {0} to {1}.", | ||
| new Object[] { validIssuerIndex, validChainEnd + 1 }); | ||
| chain.remove(validIssuerIndex); | ||
| chain.add(validChainEnd + 1, validIssuerInChain); | ||
| } else { | ||
| LOGGER.log(FINE, "Valid issuer already at correct contiguous position."); | ||
| } | ||
| // Continue the loop to potentially download the issuer's issuer | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Another guard to avoid keeping the loop going on indefinitely.
| if (validIssuerInChain != null) { | |
| // Issuer exists in the chain. If it's not in the expected position (validChainEnd+1), | |
| // move it to make the chain contiguous | |
| if (validIssuerIndex != validChainEnd + 1) { | |
| LOGGER.log(FINE, "Valid issuer found but not at contiguous position. Moving from index {0} to {1}.", | |
| new Object[] { validIssuerIndex, validChainEnd + 1 }); | |
| chain.remove(validIssuerIndex); | |
| chain.add(validChainEnd + 1, validIssuerInChain); | |
| } else { | |
| LOGGER.log(FINE, "Valid issuer already at correct contiguous position."); | |
| } | |
| // Continue the loop to potentially download the issuer's issuer | |
| continue; | |
| } | |
| if (validIssuerInChain != null) { | |
| if (validIssuerIndex > validChainEnd + 1) { | |
| // Valid issuer sits among the appended/unplaced certs after the valid prefix. | |
| // Moving it up into the contiguous slot is safe: the removal is beyond the valid | |
| // prefix, so it cannot break an earlier link. This makes forward progress, so | |
| // re-evaluate the chain from the top. | |
| LOGGER.log(FINE, "Valid issuer found but not at contiguous position. Moving from index {0} to {1}.", | |
| new Object[] { validIssuerIndex, validChainEnd + 1 }); | |
| chain.remove(validIssuerIndex); | |
| chain.add(validChainEnd + 1, validIssuerInChain); | |
| continue; | |
| } else if (validIssuerIndex <= validChainEnd) { | |
| // The matching issuer lies *inside* the already-valid prefix. This can occur with | |
| // duplicate or cross-signed intermediates that share a subject DN and key. Removing | |
| // it would break the valid prefix and could keep the loop oscillating between two | |
| // chain arrangements without ever decrementing maxDownloads, so do NOT reposition here. | |
| // Fall through to the bounded AIA download branch, which inserts a fresh issuer copy | |
| // contiguously and makes guaranteed forward progress. | |
| LOGGER.log(FINE, | |
| "Valid issuer for [{0}] found inside the valid prefix at index {1} (likely duplicate or " | |
| + "cross-signed). Not repositioning; attempting AIA download instead.", | |
| new Object[] { x509Top.getSubjectX500Principal().getName(), validIssuerIndex }); | |
| } else { | |
| // validIssuerIndex == validChainEnd + 1: already contiguous. findValidChainEnd would | |
| // normally have consumed it already; fall through rather than spinning on `continue`. | |
| LOGGER.log(FINE, "Valid issuer already at correct contiguous position."); | |
| } | |
| // Fall through to the AIA download branch below. | |
| } |
Problem
When a non-exportable DigiCert certificate in Azure Key Vault is used with
jarsigner, the signed JAR fails verification with:Fixes #44267 (regression confirmed unfixed in 2.11.0-beta.1 per this comment).
Root Cause
The Azure Key Vault
/secrets/{alias}endpoint returns only the leaf certificate for non-exportable keys (no private key, and no intermediate CA certificates when the caller only provided the leaf cert duringMergeCertificate). ThegetCertificateChain()method therefore returns a one-element array. An earlier fix (PR #41303) added intermediate cert support and PR #47977 added ordering — neither helps when the intermediate is genuinely absent.Without the intermediate CA in the chain:
jarsignerembeds only the leaf cert in the JAR signature blockjarsigner -verifycalls Java's PKIX path builder to trace leaf -> intermediate -> root CAcacerts-> path building failsSolution
After loading certificates from the AKV secret bundle, walk the chain upward. If the top certificate is not self-signed (i.e. the chain is incomplete), parse its AIA (Authority Information Access) extension (OID
1.3.6.1.5.5.7.1.1) to find thecaIssuersURL, download the missing intermediate CA certificate (DER or PEM), and append it. Repeat until the chain reaches a self-signed root CA or no AIA URL is found.This is the standard mechanism used by browsers, AzureSignTool, and major signing tools to complete certificate chains at runtime.
Example (AIA in certificate context):
In this flow, chain completion follows
CA Issuerslevel by level (leaf -> intermediate -> root) until the chain is complete or no issuer can be resolved.Implementation
Core Algorithm
findValidChainEnd()to identify true chain end (prevents interfering with extra/unplaced certs).Security Hardening
azure.keyvault.jca.disable-aia-downloadallows disabling AIA in locked-down environments.basicConstraintsor self-signed root).keyCertSignmust be set (RFC 5280 alignment).Lint and Review Follow-ups
isValidIssuerto satisfy SpotBugs (REC_CATCH_EXCEPTION).Self-Signed) with signature-based self-signed verification.findValidChainEndJavaDoc wording to match actual chain-walking direction.Changes
CertificateUtil.javacompleteChainViaAia(),downloadIssuerCertificateFromAia(),findValidChainEnd(),isSelfSignedCertificate(),isValidIssuer(); improveorderCertificateChain(); enforce KeyUsagekeyCertSign; select matching issuer from AIA bundles; improve diagnostics and lint complianceHttpUtil.javagetBytes(String url)for binary downloads (10s timeout) with exception logging for diagnosticsAiaCertificateChainTest.javaCertificateOrderTest.javaCHANGELOG.mdREADME.mdazure.keyvault.jca.disable-aia-downloadto Exposed Optionscheckstyle-suppressions.xmlCertificateUtil.java/HttpUtil.java(module continues to usejava.util.logging)Test Coverage
PKIX Path Building:
pkixPathBuildingWithoutFixFailsWithReportedError✓ Reproduces exact error from [BUG] jarsigner + jca still reports that entries in certificate chain are invalid #44267pkixPathBuildingWithFixSucceeds✓ Full chain built and PKIX validation succeedsCertificate Ordering (PEM/PKCS12):
testPemCertificateChainOrder✓ Verifies correct leaf -> intermediate -> root orderingtestPkcs12CertificateChainOrder✓ Validates PKCS12 format chain orderingtestOrderCertificateChainIncompleteRootFirst✓ Regression test: [root, leaf] input correctly orders to [leaf, root]AIA Chain Completion:
keyCertSignwhen KeyUsage present ✓azure.keyvault.jca.disable-aia-download=trueprevents AIA downloads ✓Results: 97/97 tests pass (0 failures, 29 skipped)
Customer Validation
Confirmed working by the original reporter (@manfrede) in #44267 (comment) using
azure-security-keyvault-jca-2.12.0-beta.1.jar. The AIA chain completion path executed correctly as shown in debug logs:jarsigner produced no warnings and the full chain was embedded correctly.
Backward Compatibility